-
Notifications
You must be signed in to change notification settings - Fork 379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(make): fix stripped directory names with bind 'set show-all-if-ambiguous on'
#546
fix(make): fix stripped directory names with bind 'set show-all-if-ambiguous on'
#546
Conversation
Added a few rather superficial comments. The
This sounds good to me in principle, but I can't tell offhand what it would mean to user experience.
Do we think that Makefiles can get sufficiently large to make efficiency here a issue we need to care much about? To me, code understandability and portability are more important unless there is an actual performance issue we do want to tackle, and in general bash wins over sed or awk on the understandabilty/portability front. But things can get hairy and overly verbose with bash, too, so this is something to look into case by case. |
Removing the dependence on COMP_TYPE means that we effectively revert the feature introduced by 39f00f9. This feature strips the directory name in the list of completions displayed in the terminal. For example, when we try to complete
I think basically a handwritten Makefile shouldn't be so large (unless the user uses fancy GNU make macros to automatically generate a bunch of rules). However, the result of Makefile generators such as autotools and cmake can be extremely large.
For understandability, I believe
Maybe I should make a sample implementation by |
Sure, if you're interested. While at it, no matter the language, I think it would be cool if we could extract the whole implementation to a separately invoked static script in |
I'm going to update this PR after another PR #548, which the new commits for this PR would depend on, is processed.
If you are fine with removing the COMP_TYPE-dependent feature 39f00f9, I can remove it. What do you think?
I would do it in another PR if I have time after this PR is settled. |
bind 'set show-all-if-ambiguous on'
bind 'set show-all-if-ambiguous on'
bind 'set show-all-if-ambiguous on'
bind 'set show-all-if-ambiguous on'
2d0f691
to
49ba02e
Compare
49ba02e
to
89cfbaf
Compare
I still hesitate to choose the solution for this issue. In the current test framework, the completions are extracted with Isn't there any way to update the test framework so that it can handle different cases of [Edit: Sorry there were many typos of |
This is an area in which I have little experience, so unsure how much I can be of assistance. A couple of (likely stupid) comments/questions though:
Not sure I follow -- the only
Doesn't strike me as impossible off the bat, but nothing concrete to offer right now either. |
89cfbaf
to
5bc3823
Compare
5bc3823
to
edc0bbd
Compare
edc0bbd
to
5b23b5f
Compare
We might also need to exclude the case |
bind 'set show-all-if-ambiguous on'
bind 'set show-all-if-ambiguous on'
5b23b5f
to
9f03119
Compare
9f03119
to
e204c78
Compare
85ffc5b
to
d21ebba
Compare
The issue is reported again in #858. I think there is no reason to block the fixes just for the test suite problem. Here, I have rebased it on top of the latest master and, in commit d21ebba, deactivated the display-only mode (39f00f9) causing the test-suite issue. If we would like to still use the "display-only mode", we can discuss how to make it work with the test framework in a separate issue, but I'm currently not sure if the "display-only mode" is worth re-enabling. |
In the display-only mode where COMP_TYPE=9, 37, or 42, only a part of the completion is stored in COMPREPLY for displaying purposes. For example, "xyz" is stored in COMPREPLY when "abc/xyz" is the candidate and "abc/" is already inserted. However, the test framework extracts generated completions using COMP_TYPE=37 by setting "set show-all-if-ambiguous off", which would be broken by the display-only mode. There is no simple way to make it work with the test framework, and the display-only mode does not seem to be essential. For the time being, we deactivate the display-only mode. See also discussions in the following links: scop#544 scop#546
d21ebba
to
5df3eef
Compare
In the display-only mode where COMP_TYPE=9, 37, or 42, only a part of the completion is stored in COMPREPLY for displaying purposes. For example, "xyz" is stored in COMPREPLY when "abc/xyz" is the candidate and "abc/" is already inserted. However, the test framework extracts generated completions using COMP_TYPE=37 by setting "set show-all-if-ambiguous off", which would be broken by the display-only mode. There is no simple way to make it work with the test framework, and the display-only mode does not seem to be essential. For the time being, we deactivate the display-only mode. See also discussions in the following links: scop#544 scop#546
5df3eef
to
3d8811b
Compare
Can confirm this works fine. Not sure why we need all that code though, bash should be able to do the right thing with paths ( |
Thank you for checking!
I guess you mean The reason for not using Bash's filename generation is that we would like to generate the target names defined in |
I am talking about processing, not generation. I guess what we're doing isn't even specific to paths. |
OK, I see. Thank you for your clarification! What we are doing here is not equivalent to P.S.
It seems the reduction is introduced in commit b28d710. This seems to predate the time we moved to GitHub, so I don't know where to find the corresponding discussion. |
I mean, leaving aside In other words this "reduction" behaviour you're talking about is just... how completion works, why would we need to reimplement it ourselves for make? |
Ah, I see what you mean now. I have been considering the case where there are multiple directories and multiple files therein and we do not want to list the entire file tree by But maybe the behavior you suggest makes sense. |
Ah, I see what you mean, we're reducing the list of options shown. That makes sense. |
It's hard to decide for me. This is related to the background that introduced this line ten years ago. This is just my guess, but people might have wanted to mimic the behavior of Bash for normal paths. |
In the display-only mode where COMP_TYPE=9, 37, or 42, only a part of the completion is stored in COMPREPLY for displaying purposes. For example, "xyz" is stored in COMPREPLY when "abc/xyz" is the candidate and "abc/" is already inserted. However, the test framework extracts generated completions using COMP_TYPE=37 by setting "set show-all-if-ambiguous off", which would be broken by the display-only mode. There is no simple way to make it work with the test framework, and the display-only mode does not seem to be essential. For the time being, we deactivate the display-only mode. See also discussions in the following links: scop#544 scop#546
e445428
to
a16a6b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not delay this any longer
As noted, the make completion is already too complex for my taste and this makes it even more complex. Then again, we have the test cases here which appear to make sense as well as good amount of commentary, and no alternative implementations for taking care of the issue, so leaving it up to @akinomyoga to merge if he's comfortable with what we have here. |
In the display-only mode where COMP_TYPE=9, 37, or 42, only a part of the completion is stored in COMPREPLY for displaying purposes. For example, "xyz" is stored in COMPREPLY when "abc/xyz" is the candidate and "abc/" is already inserted. However, the test framework extracts generated completions using COMP_TYPE=37 by setting "set show-all-if-ambiguous off", which would be broken by the display-only mode. There is no simple way to make it work with the test framework, and the display-only mode does not seem to be essential. For the time being, we deactivate the display-only mode. See also discussions in the following links: scop#544 scop#546
a16a6b5
to
36fb9bb
Compare
When many targets in subdirectories are defined in Makefile, we typically want to list the files or subdirectories in the current directory. However, when the file in the subdirectory is unique, we may generate the full path. See also the discussion on GitHub: scop#544 scop#546 (comment)
In the display-only mode where COMP_TYPE=9, 37, or 42, only a part of the completion is stored in COMPREPLY for displaying purposes. For example, "xyz" is stored in COMPREPLY when "abc/xyz" is the candidate and "abc/" is already inserted. However, the test framework extracts generated completions using COMP_TYPE=37 by setting "set show-all-if-ambiguous off", which would be broken by the display-only mode. There is no simple way to make it work with the test framework, and the display-only mode does not seem to be essential. For the time being, we deactivate the display-only mode. See also discussions in the following links: scop#544 scop#546
36fb9bb
to
5eb1042
Compare
Resolve #544
@pytest.mark.bashcomp(required_cmd=True)
to skip all the tests in the class.COMP_TYPE=37
. This happens when the user presses TAB under the readline settingbind 'set show-all-if-ambiguous on'
is set.Test suite problem on COMP_TYPE
The test
test_github_issue_544_4
(test_make.py
) currently fails because of the design of the test suite. The test suite extracts the generated completions by changing the readline settings asUnder these settings, the completion functions are called with
COMP_TYPE=33
(!
, show list), which is different from the usualCOMP_TYPE=9
(TAB) orCOMP_TYPE=37
(%
, menu-complete). The completion formake
changes the behavior depending onCOMP_TYPE
so that it doesn't produce the expected results in the test suite. Note thatmake
completion is the only one that changes its behavior depending on the value ofCOMP_TYPE
among the completions inbash-completion
.@scop Do you have any thoughts on how we should solve this problem?
COMP_TYPE
?completions/make
that detects the test suite and changes the behavior only in the test suite?Possibility of using awk
sed
has been used to extract the target names bymake
. It is difficult to usesed
to test whether the filenames in subdirectories are unique and to generate the full path only when it is unambiguous. In commit d5181c5,sed
scripts has been changed so as to always produce the full paths, and instead, the generated results are afterward filtered using Bash code. But, maybe another approach is to rewrite everything usingawk
. It should be more efficient than processing in Bash.